fix: edge cases around mismatched path in json code #3609

romange · 2024-08-30T16:12:43Z

For legacy mode:

For mutate commands, an empty result should throw an error
For read commands, it returns nil if path was not found, but if it was matched but only with a wrong types, it will throw an error.

For non-legacy mode, objlen should throw an error for non existing key.

Simplified JsonCallbackResult a bit and made sure more fakeredis tests are passing.

For legacy mode: 1. For mutate commands, an empty result should throw an error 2. For read commands, it returns nil if path was not found, but if it was matched but only with a wrong types, it will throw an error. For non-legacy mode, objlen should throw an error for non existing key. Simplified JsonCallbackResult a bit and made sure more fakeredis tests are passing. Signed-off-by: Roman Gershman <roman@dragonflydb.io>

BagritsevichStepan · 2024-09-02T12:32:51Z

src/server/json_family_test.cc

  resp = Run({"JSON.STRLEN", "json"});
  EXPECT_THAT(resp, ErrArg("wrong JSON type of path value"));

  resp = Run({"JSON.OBJLEN", "json", ".a"});
  EXPECT_THAT(resp, IntArg(0));

  resp = Run({"JSON.OBJLEN", "json", ".a.*"});
-  EXPECT_THAT(resp, ErrArg("wrong JSON type of path value"));
+  EXPECT_THAT(resp, ArgType(RespExpr::NIL));


In Redis I'm getting WRONGTYPE error:

127.0.0.1:6379> json.set json . '{"a":{"b":"c"}}' OK 127.0.0.1:6379> json.objlen json .a.* (error) WRONGTYPE wrong type of path value - expected object but found string

Same with this PR but it depends if there was a match or not. With this test the input is:
{"a":{}, "b":{"a":"a"}, "c":{"a":"a", "b":"bb"}, "d":{"a":1, "b":"b", "c":{"a":3,"b":4}}, "e":1} and here there are 0 matches, hence NIL

BagritsevichStepan · 2024-09-02T12:37:02Z

src/server/json_family_test.cc

@@ -656,7 +658,7 @@ TEST_F(JsonFamilyTest, ToggleLegacy) {
  ASSERT_THAT(resp, "OK");

  resp = Run({"JSON.TOGGLE", "json"});
-  EXPECT_THAT(resp, ErrArg("wrong JSON type of path value"));
+  EXPECT_THAT(resp, ErrArg("wrong number of arguments"));


Interesting, the specification says that the path is an optional argument. But Redis also throws an error for the empty path

romange requested a review from BagritsevichStepan August 30, 2024 16:12

romange force-pushed the Pr1 branch from 44b291b to 3a486f3 Compare August 30, 2024 16:37

romange force-pushed the Pr1 branch from 3a486f3 to 13cca10 Compare August 30, 2024 18:37

BagritsevichStepan reviewed Sep 2, 2024

View reviewed changes

romange requested a review from BagritsevichStepan September 2, 2024 14:34

BagritsevichStepan approved these changes Sep 2, 2024

View reviewed changes

romange merged commit 879f295 into main Sep 2, 2024
10 checks passed

romange deleted the Pr1 branch September 2, 2024 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: edge cases around mismatched path in json code #3609

fix: edge cases around mismatched path in json code #3609

romange commented Aug 30, 2024

BagritsevichStepan Sep 2, 2024

romange Sep 2, 2024

BagritsevichStepan Sep 2, 2024

fix: edge cases around mismatched path in json code #3609

fix: edge cases around mismatched path in json code #3609

Conversation

romange commented Aug 30, 2024

BagritsevichStepan Sep 2, 2024

Choose a reason for hiding this comment

romange Sep 2, 2024

Choose a reason for hiding this comment

BagritsevichStepan Sep 2, 2024

Choose a reason for hiding this comment